-
Notifications
You must be signed in to change notification settings - Fork 11.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds multi-commit checkpoint batching in Sui. #17955
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
7731d23
to
6ced907
Compare
Adds `version_specific_data` to `CheckpointSummary` to keep track of which `RandomnessRound`s are present in a checkpoint. Batching is configurable by a minimum interval based on the commit timestamp.
6ced907
to
198147b
Compare
c6e029c
to
59129e8
Compare
bbd70b8
to
ff5797a
Compare
Semgrep found 1
Risk: Affected versions of vite are vulnerable to Improper Handling Of Case Sensitivity / Exposure Of Sensitive Information To An Unauthorized Actor / Improper Access Control. The vulnerability arises when the Vite development server's option, Manual Review Advice: A vulnerability from this advisory is reachable if you host vite's development server on Windows, and you rely on Fix: Upgrade this library to at least version 4.5.2 at sui/examples/trading/frontend/pnpm-lock.yaml:4700. Reference(s): GHSA-c24v-8rfc-w8vw, CVE-2023-34092, CVE-2024-23331 Ignore this finding from ssc-efa14576-9601-4ae6-939c-3da58aa25013. |
None | Some(0) => None, | ||
Some(1) => Some( | ||
bcs::from_bytes(&self.version_specific_data).unwrap_or_else(|e| { | ||
panic!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is not on a CertifiedCheckpointSummary, we should probably return an error instead of panicking. Caller can panic if the summary was certified (and verified!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just some minor comments below.
debug!( | ||
checkpoint_commit_height = height, | ||
"Making checkpoint at commit height" | ||
); | ||
if let Err(e) = self.make_checkpoint(height, pending).await { | ||
if let Err(e) = self | ||
.make_checkpoint(std::mem::take(&mut grouped_pending_checkpoints)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit tricky to see through all the cases where grouped_pending_checkpoint still has leftovers. Maybe we should consider cleaning it up later.
min_checkpoint_interval_ms: Option<u64>, | ||
|
||
/// Version number to use for version_specific_data in `CheckpointSummary`. | ||
checkpoint_summary_version_specific_data: Option<u64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would this be an enum in feature flag? (to get rid of all the unimplemented!()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had it as a feature flag and @mystenmark asked me to use this style instead, other "version" settings in the protocol config also just use ints e.g. move_binary_format_version
, gas_model_version
, execution_version
let version_specific_data = match protocol_config | ||
.checkpoint_summary_version_specific_data_as_option() | ||
{ | ||
None | Some(0) => Vec::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here (or somewhere), I think we also need to check that when checkpoint_summary_version_specific_data
is < 0, min_checkpoint_interval_ms
must be 0 . Otherwise, if checkpoint_summary_version_specific_data < 1, and min_checkpoint_interval_ms > 0, randomness rounds may be missed here.
Description
Adds
version_specific_data
toCheckpointSummary
to keep track of whichRandomnessRound
s are present in a checkpoint.Batching is configurable by a minimum interval based on the commit timestamp.
Test plan
Unit/integration tests, manual testing in synthetic environment.
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.